Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Un-ossify the build system #36

Merged
merged 119 commits into from
Jan 21, 2025
Merged

Conversation

tstirrat15
Copy link
Contributor

@tstirrat15 tstirrat15 commented Jan 10, 2025

Description

This escalated quickly. Roughly: I started by just wanting to collapse the workspaces. In moving the yarn lockfiles around, I triggered an upgrade of one package or another, which moved to ESM, which the create-react-app version of Webpack wasn't able to parse. You're kinda stuck with what CRA gives you as far as versions of things go.

That prompted a move to Vite, which shouldn't have been that destructive a change, except that we were doing a bunch of non-standard stuff. I had to fix a bunch of typescript issues that came with the new typescript version, which also included upgrading how we're doing protobuf generation. I also simplified the build process as much as I could by moving things into the public dir and letting vite copy them around.

Changes

I'm gonna annotate this to call out the most relevant bits.

Testing

See that things are green and check out the preview to see that it works as desired.

Copy link

vercel bot commented Jan 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 21, 2025 8:52pm

@tstirrat15 tstirrat15 changed the title Flatten workspaces Un-ossify the build system Jan 16, 2025
Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See annotations

.dockerignore Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to a gitignore file and tells the docker runtime to leave all of these files/folders out of the build context. It means we can simplify the dockerfile.

@@ -1,30 +1,15 @@
ARG BASE_IMAGE=node:18-alpine3.18
ARG BASE_IMAGE=node:22-alpine
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get to the most recent LTS

Comment on lines -5 to +6
COPY ./package.json .
COPY ./yarn.lock .
COPY ./playground-ui ./playground-ui
COPY ./spicedb-common ./spicedb-common
COPY ./playground/package.json ./playground/package.json
# Bring in everything not ignored by the dockerignore
COPY . .
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to be specific when we have dockerignore

ENV YARN_CACHE_FOLDER=/tmp/yarn_cache
RUN yarn install --frozen-lockfile --production --non-interactive --network-timeout 1000000
RUN yarn install --frozen-lockfile --non-interactive --network-timeout 1000000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We explicitly want dev dependencies in the build process - otherwise we're leaving out things like typescript and our bundler.

Comment on lines -23 to -27
RUN npm install -g jshint
COPY ./playground/contrib/generate-config-env.sh .
COPY ./playground/contrib/test-config-env.sh .
RUN ./test-config-env.sh
RUN echo 'Config Verified' > verified
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config isn't used anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got tests working with Vitest, which is faster and less janky than Jest. Vitest has you import the testing functions rather than treating them as implicit globals, so all test files have these changes.

const possibleObjectTypes = [];
possibleObjectTypes.push(...resourceTypes);
possibleObjectTypes.push(...subjectTypes);
const possibleObjectTypes = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixed a typescript issue.


describe("parsing validation YAML", () => {
it("returns undefined for an empty file", () => {
expect(parseValidationYAML("")).toEqual({
message: "data must be object",
message: "data should be object",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing these tests stopped passing on a recent SpiceDB wasm upgrade, but we don't run the tests in CI so we missed it. I'm going to enable tests as a follow-on to this.

@@ -0,0 +1,17 @@
/// <reference types="vite/client" />

interface ImportMetaEnv {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes typescript happier about import.meta.env variables and acts as a listing of the environment variables used in the application.

plugins: [react(), svgr(),],
build: {
// This matches Vercel's expectations.
outDir: "build",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vite defaults to putting things in dist, so we modify that here so that we don't have to muck with Vercel settings.

Copy link

@alecmerdler alecmerdler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went over these changes in a huddle, looks good. Waiting on a couple follow-ups before approving.

Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For myself

vercel.json Outdated
Comment on lines 55 to 63
{
"source": "/s/:path([^/]+)/download",
"headers": [
{
"key": "Content-Security-Policy",
"value": "frame-ancestors 'self' docs.authzed.com authzed.com www.authzed.com;"
}
]
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reinstate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@tstirrat15 tstirrat15 marked this pull request as ready for review January 16, 2025 21:02
alecmerdler
alecmerdler previously approved these changes Jan 17, 2025
Copy link

@alecmerdler alecmerdler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this!

README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For docker builds, add note to specify all the env vars or add default values to the dockerfile

README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the NODE_OPTIONS line from the vercel deploy option section (line 61) and node option (line 76).
Update instructions in the "Running for development" section

Dockerfile Outdated

COPY ./playground ./playground
# App environment variables
ENV VITE_AUTHZED_DEVELOPER_GATEWAY_ENDPOINT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These env vars can all be defaulted to empty string

Copy link
Member

@samkim samkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tstirrat15 tstirrat15 merged commit 03f126d into main Jan 21, 2025
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants